-
Notifications
You must be signed in to change notification settings - Fork 301
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HPCC-29838 Add false sharing test case #17515
HPCC-29838 Add false sharing test case #17515
Conversation
https://track.hpccsystems.com/browse/HPCC-29838 |
Do you get results similar to this ? This might actually hide some sharing effects due to a lock xadd instruction ? |
My results were (on a debug build) 16:23:28.297867 259 Specified library location sdf not found using a 3.6 GHz 8-Core Intel Core i9 (not dual-socket) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved.
Perhaps also interesting if near and far non-atomic increment were added as well.
@richardkchapman I haven't merged because you were making other changes (e.g. local reference within the callbacks). Let me know if you want me to merge as-is. |
9f705ff
to
1fcc691
Compare
@richardkchapman let me know if your happy for it be merged (I will take Gavins comment as an approval!). |
roxie/ccd/ccdsnmp.cpp
Outdated
RelaxedAtomic<unsigned> retriesIgnoredSec; | ||
RelaxedAtomic<unsigned> retriesNeeded; | ||
RelaxedAtomic<unsigned> retriesReceivedPrm; | ||
RelaxedAtomic<unsigned> retriesIgnoredPrm __attribute__((aligned(CACHE_LINE_SIZE))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change should probably not be included
system/jlib/jatomic.hpp
Outdated
@@ -51,7 +51,7 @@ auto sub_fetch(T & value, decltype(value.load()) delta, std::memory_order order | |||
//NOTE: Counts will never be lost, but the values read from another thread may be inconsistent. | |||
//E.g., thread 1 updates x than y, thread 2 may read an updated value of y, but an old value of x. | |||
template <typename T> | |||
class RelaxedAtomic : public std::atomic<T> | |||
class alignas(CACHE_LINE_SIZE) RelaxedAtomic : public std::atomic<T> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nor this one
1fcc691
to
5b6e274
Compare
I removed a couple of changes that should not really have been included. As far as I am concerned it's now ok to merge, but not in any way urgent. |
@mckellyln can you re-review latest changes? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am fine with this PR except I think the java changes are not related and perhaps would go away with a rebase ?
Signed-off-by: Richard Chapman <[email protected]>
5b6e274
to
2e40cea
Compare
@mckellyln I have repushed to remove the unwanted Java changes |
Type of change:
Checklist:
Smoketest:
Testing: